(feat) O3-3861 - ward app - add tooltip to obs to show encounter date #1312
Conversation
chibongho
requested review from
mseaton,
mogoodrich,
vasharma05 and
usamaidrsk
|
Size Change: -33.8 kB (-0.55%) Total Size: 6.1 MB
ℹ️ View Unchanged
|
| @@ -44,7 +38,7 @@ export function useMotherAndChildren( | |||
| requireChildBornDuringMothersActiveVisit?.toString() ?? 'false', | |||
| ); | |||
| rep && url.searchParams.append('v', rep); | |||
| return useOpenmrsPagination<MotherAndChildren>(fetch ? url : null, 50); | |||
| return useOpenmrsFetchAll<MotherAndChildren>(fetch ? url : null); | |||
There was a problem hiding this comment.
Sneaking this fix in. This shouldn't have use useOpenmrsPagination.
| @@ -62,16 +68,6 @@ const WardPatientCard: WardPatientCard = (wardPatient) => { | |||
| ))} | |||
| <ExtensionSlot name={footerExtensionSlotName} state={wardPatient} /> | |||
| </div> | |||
| <button | |||
There was a problem hiding this comment.
Previously, the patient card was made clickable by adding this button to overlay on top of the elements of the card. I'm not sure why it was implemented that way, but I had to change it in order for the ToggleTips to receive click events.
cc @brandones
There was a problem hiding this comment.
It is widely and strongly recommended against adding onClick to a div.
There are myriad articles about how to create a "clickable card" component, which is simply kind of a hard problem, but I really like this one. I implemented Method 3 from it, which agrees with the recommendations of the widely-cited Card guidelines from Inclusive Components.
Rather than using JS as recommended by Method 4 of that article, I would recommend just using the current approach and increasing the z-index of any links you need to be clickable on top of the card. The only reason Method 4 would be preferable would be to make the text selectable, and I don't think that's relevant to this interface.
As an aside, did this ToggleTip thing get designed? It makes the interface look a lot busier, appears to have the info button at the same level of visual hierarchy as the obs information itself, and seems like it would be easy to click the wrong thing. I would suggest making the tooltip appear on hover anywhere over the tag element (with a 500ms delay), with no info icon. You would only need JS to do touch handling on mobile, something like touch-and-hold shows the tooltip while tap clicks the card. You would still need the z-index fix.
Alternatively, since it seems like this is supposed to solve a problem not actually faced by users (but rather by devs & admins) you could add a boolean config option that makes all tags show some metadata. Then you would be able to debug just by switching it in the implementer tools.
There was a problem hiding this comment.
Ok, makes sense. I'll implement it using z-index.
As an aside, did this ToggleTip thing get designed?
No, I did this before I got a reply from UX designers. I'll make the whole tag clickable for mobile and hoverable on desktop.
Alternatively, since it seems like this is supposed to solve a problem not actually faced by users (but rather by devs & admins)
While it's mostly for debugging, I think it's also important to show this info so that mistakes in implementation / configuration are as apparent as possible to the end users.
There was a problem hiding this comment.
Thanks @chibongho ! Looks good, though one comment on formatting the display information.
| <ToggletipButton> | ||
| <Information /> | ||
| </ToggletipButton> | ||
| <ToggletipContent>{o.encounter?.display}</ToggletipContent> |
There was a problem hiding this comment.
Note: we generally don't use the "display" property much because we often want finer-grained control of what to "display". For one thing, "display" does not support our localization strategy for metadata (though it likely should), but also, this will return the time formatted in the server time zone, which was our general standard for O2, but probably not what we want in O3? ie, it's probably better to explicitly format the encounter type and encounter date here... thoughts @ibacher @mseaton ?
| } else { | ||
| summaryTagTooltipText.push( | ||
| <div key={uuid}> | ||
| {display} ({o.encounter.display}) |
| <ToggletipButton> | ||
| <Information /> | ||
| </ToggletipButton> | ||
| <ToggletipContent>{o.encounter?.display}</ToggletipContent> |
|
From the screenshots, it looks like the Toggletips could benefit from some left margin. |
Oh... I should have mentioned. The screenshots are not up to date anymore. I'm making the entire tags be a tooltip on hover when on desktop and a toggletip when on mobile. It looks like this now:
|
Requirements
Summary
The observations displayed in the ward patient cards current has no indication to show when the obs values were taken. If the values were mis-configured to show from previous visits then the values might not be accurate.
This PR introduces ToggleTips that allow user to click on the obs values and see the corresponding encounter date.
Screenshots
Related Issue
https://openmrs.atlassian.net/browse/O3-3861
Other